-
Notifications
You must be signed in to change notification settings - Fork 189
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
feat(cli): improve storeArgument, refactor cli #500
Conversation
Co-authored-by: alvarius <[email protected]>
valueSchema.validate(false); | ||
keySchema.validate(true); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm not sure if the following compiles, but it would be more readable in this case. Should test if it compiles before committing though
valueSchema.validate(false); | |
keySchema.validate(true); | |
valueSchema.validate({ allowEmpty: false }); | |
keySchema.validate({ allowEmpty: true }); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
haha you merged it right before I pushed the changes. It does compile
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
(wanted to do as a manual commit since it's used in 2 other places)
// tableIdArgument: true, | ||
// }, | ||
Bool: { | ||
// TODO: This table is only used for testing, move it to `test/tables` via the directory config once supported |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
this should be supported now right?
// TODO: This table is only used for testing, move it to `test/tables` via the directory config once supported | |
directory: "../test/tables", |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
there're some problems with just doing this, so I left it for another PR, this one is big enough as it is
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think forge doesn't see imports in test
, unless we add test
to libs
, which is a questionable decision and I haven't looked into what to really do about it yet
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I see, fine to leave it for later!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
(like me, you might wonder how it works in solecs
- because there test
is in src/test
)
}, | ||
storeArgument: true, | ||
tableIdArgument: true, | ||
}, | ||
AddressArray: { | ||
// TODO: This table is only used for testing, move it to `test/tables` via the directory config once supported |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
same here
// TODO: This table is only used for testing, move it to `test/tables` via the directory config once supported | |
directory: "../test/tables", |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
lgtm!
|
||
export function parseStoreConfig(config: StoreUserConfig) { | ||
export function parseStoreConfig(config: unknown) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Did this have to get reverted?
Nevermind, I don't think this impacted downstream types like I thought it would!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
this was an accidental revert actually - was adapting these changes from code before your PR and missed this. But good to know all works well
A bunch of code improvements for solidity renderers and store config
And some fixes:
this is extracted from #459